-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-10775. Support bucket ownership verification #8558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
peterxcli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hevinhsu for this patch!
I think we could also add robot tests to each s3 request smoke tests (excluding bucketcreate.robot and bucketlist.robot)
...-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketOwnerCondition.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/ozone/s3/awssdk/v2/AbstractS3BucketVerificationConditionUsingS3SDKV2Tests.java
Outdated
Show resolved
Hide resolved
...-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketOwnerCondition.java
Outdated
Show resolved
Hide resolved
...s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestMultipartUploadComplete.java
Outdated
Show resolved
Hide resolved
|
@peterxcli Thanks for the review. I'll add tests to each robot test file. |
ivandika3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hevinhsu Thanks for working on this. Left a few comments.
...-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketOwnerCondition.java
Outdated
Show resolved
Hide resolved
...-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketOwnerCondition.java
Outdated
Show resolved
Hide resolved
...-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketOwnerCondition.java
Outdated
Show resolved
Hide resolved
...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java
Outdated
Show resolved
Hide resolved
|
Thank to @peterxcli for pointing out the need to add smoke tests! To keep this PR focused, may I open a new issue to implement the S3 request smoke tests? |
|
A new JIRA issue for the smoke test has been opened: HDDS-13260. |
|
Hi @ivandika3 @peterxcli , I've made some changes, including:
Would you mind taking a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hevinhsu for the continued effort on this patch and the extensive tests. Overall looks good. I left some minor comments (mostly some nits), after they are addressed, I think this patch is good to go.
The main follow up we need to do is reduce the additional getBucket OM call introduced in this patch. In addition to the comments, we can think about a way to only call getBucket if the x-amz-expected-bucket-owner or x-amz-source-expected-bucket-owner is specified so that normal requests without expected bucket owner won't see increased latency. This would make the code slightly more complicated, but would save us some RTT.
...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java
Outdated
Show resolved
Hide resolved
...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java
Outdated
Show resolved
Hide resolved
...gration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java
Outdated
Show resolved
Hide resolved
| bucket = getBucket(bucketName); | ||
| S3Owner.verifyBucketOwnerCondition(hh, bucketName, bucket.getOwner()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since getBucket is pushed up, we can pass the OzoneBucket into the getAcl and listMultipartUploads instead to prevent multiple duplicate getBucket calls.
We can address this in a follow up ticket.
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java
Outdated
Show resolved
Hide resolved
| throw newError(NOT_IMPLEMENTED, keyPath); | ||
| } | ||
| OzoneVolume volume = getVolume(); | ||
| OzoneBucket bucket = volume.getBucket(bucketName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment to remove getBucket duplication. Can be addressed in a follow up.
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java
Outdated
Show resolved
Hide resolved
…aders on each of the function,
…y checked in caller method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hevinhsu Thanks for the update. One more comment I missed.
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java
Outdated
Show resolved
Hide resolved
|
@ivandika3 Thanks again for the great suggestions — most of them have now been addressed! I initially chose to use a static method for bucket owner validation because many APIs already require an OzoneBucket, and I felt this approach would minimize the amount of changes needed in the current codebase. That said, I really appreciate your point about reducing the getBucket call to improve RTT. I'll give it some thought and explore how we might optimize this further. As for filing a follow-up issue, I'm not quite sure how to scope it yet to ensure the changes stay manageable. If you have suggestions on how to break it down or where it might fit best, I'd be happy to collaborate on that. |
ivandika3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements and updates so far. LGTM +1.
As for filing a follow-up issue, I'm not quite sure how to scope it yet to ensure the changes stay manageable.
We can simply raise a ticket to reduce the getBucket info first. We can split them into multiple tasks to make them more manageable if needed.
|
Thanks for the review! I've created the follow-up Jira issue here: HDDS-13270. |
peterxcli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hevinhsu updating the patch. LGTM!
I have two minor suggestion about simplifying the logic in S3Owner, but I think they can be addressed in follow-up, too.
| public static void verifyBucketOwnerCondition(HttpHeaders headers, String bucketName, String bucketOwner) | ||
| throws OS3Exception { | ||
| if (headers == null || bucketOwner == null) { | ||
| return; | ||
| } | ||
|
|
||
| final String expectedBucketOwner = headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER); | ||
|
|
||
| if (StringUtils.isEmpty(expectedBucketOwner)) { | ||
| return; | ||
| } | ||
| if (expectedBucketOwner.equals(bucketOwner)) { | ||
| return; | ||
| } | ||
| throw S3ErrorTable.newError(S3ErrorTable.BUCKET_OWNER_MISMATCH, bucketName); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| public static void verifyBucketOwnerCondition(HttpHeaders headers, String bucketName, String bucketOwner) | |
| throws OS3Exception { | |
| if (headers == null || bucketOwner == null) { | |
| return; | |
| } | |
| final String expectedBucketOwner = headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER); | |
| if (StringUtils.isEmpty(expectedBucketOwner)) { | |
| return; | |
| } | |
| if (expectedBucketOwner.equals(bucketOwner)) { | |
| return; | |
| } | |
| throw S3ErrorTable.newError(S3ErrorTable.BUCKET_OWNER_MISMATCH, bucketName); | |
| } | |
| public static void verifyBucketOwnerCondition(HttpHeaders headers, String bucketName, String headerKey, String actualOwner) | |
| throws OS3Exception { | |
| if (headers == null || actualOwner == null) { | |
| return; | |
| } | |
| final String expectedOwner = headers.getHeaderString(headerKey); | |
| if (StringUtils.isEmpty(expectedOwner)) { | |
| return; | |
| } | |
| if (!expectedOwner.equals(actualOwner)) { | |
| throw S3ErrorTable.newError(S3ErrorTable.BUCKET_OWNER_MISMATCH, bucketName); | |
| } | |
| } | |
| public static void verifyBucketOwnerCondition(HttpHeaders headers, String bucketName, String actualOwner) throws OS3Exception { | |
| verifyBucketOwnerCondition(headers, bucketName, S3Consts.EXPECTED_BUCKET_OWNER_HEADER, actualOwner); | |
| } | |
| public static void verifyBucketOwnerConditionOnCopyOperation(HttpHeaders headers, String sourceBucketName, | ||
| String sourceOwner, String destBucketName, | ||
| String destOwner) | ||
| throws OS3Exception { | ||
| if (headers == null) { | ||
| return; | ||
| } | ||
|
|
||
| final String expectedSourceOwner = headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER); | ||
| final String expectedDestOwner = headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER); | ||
|
|
||
| if (expectedSourceOwner != null && sourceOwner != null && !sourceOwner.equals(expectedSourceOwner)) { | ||
| throw S3ErrorTable.newError(S3ErrorTable.BUCKET_OWNER_MISMATCH, sourceBucketName); | ||
| } | ||
|
|
||
| if (expectedDestOwner != null && destOwner != null && !destOwner.equals(expectedDestOwner)) { | ||
| throw S3ErrorTable.newError(S3ErrorTable.BUCKET_OWNER_MISMATCH, destBucketName); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| public static void verifyBucketOwnerConditionOnCopyOperation(HttpHeaders headers, String sourceBucketName, | |
| String sourceOwner, String destBucketName, | |
| String destOwner) | |
| throws OS3Exception { | |
| if (headers == null) { | |
| return; | |
| } | |
| final String expectedSourceOwner = headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER); | |
| final String expectedDestOwner = headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER); | |
| if (expectedSourceOwner != null && sourceOwner != null && !sourceOwner.equals(expectedSourceOwner)) { | |
| throw S3ErrorTable.newError(S3ErrorTable.BUCKET_OWNER_MISMATCH, sourceBucketName); | |
| } | |
| if (expectedDestOwner != null && destOwner != null && !destOwner.equals(expectedDestOwner)) { | |
| throw S3ErrorTable.newError(S3ErrorTable.BUCKET_OWNER_MISMATCH, destBucketName); | |
| } | |
| } | |
| public static void verifyBucketOwnerConditionOnCopyOperation(HttpHeaders headers, String sourceBucketName, String sourceOwner, String destBucketName, String destOwner) throws OS3Exception { | |
| if (headers == null) { | |
| return; | |
| } | |
| verifyBucketOwnerCondition(headers, sourceBucketName, S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER, sourceOwner); | |
| verifyBucketOwnerCondition(headers, destBucketName, S3Consts.EXPECTED_BUCKET_OWNER_HEADER, destOwner); | |
| } |
|
Merged. Thanks @hevinhsu and @ivandika3 for the continuous effort! |
|
Thanks @ivandika3 @peterxcli for your suggestions and review. |
) * HDDS-13296. Integration check always passes due to missing output * HDDS-10775. Fix TestS3GatewayMetrics due to #8558 * HDDS-13216. Fix TestCloseContainerCommandHandler due to #8599 * HDDS-13107. Fix TestListInfoSubcommand due to #8595 * HDDS-12873. Fix TestReplicationSupervisor due to #8305 * HDDS-12873. Fix TestReconUtils due to #8305 * HDDS-13236. Fix test failures due to #8645 * HDDS-13181. Fix TestOzoneManagerListVolumesSecure due to #8606 * HDDS-13300. Mark TestOMRatisSnapshots#testInstallIncrementalSnapshot* as unhealthy * HDDS-13302. Mark TestFSORepairTool as unhealthy * HDDS-13303. Mark TestSnapshotDeletingServiceIntegrationTest as unhealthy
…239-container-reconciliation Commits: 62 da53b5b HDDS-13299. Fix failures related to delete (apache#8665) 8c1b439 HDDS-13296. Integration check always passes due to missing output (apache#8662) 7329859 HDDS-13023. Container checksum is missing after container import (apache#8459) a0af93e HDDS-13292. Change `<? extends KeyValue>` to `<KeyValue>` in test (apache#8657) f3050cf HDDS-13276. Use KEY_ONLY/VALUE_ONLY iterator in SCM/Datanode. (apache#8638) e9c0a45 HDDS-13262. Simplify key name validation (apache#8619) f713e57 HDDS-12482. Avoid using CommonConfigurationKeys (apache#8647) b574709 HDDS-12924. datanode used space calculation optimization (apache#8365) de683aa HDDS-13263. Refactor DB Checkpoint Utilities. (apache#8620) 97262aa HDDS-13256. Updated OM Snapshot Grafana Dashboard to reflect metric updates from HDDS-13181. (apache#8639) 9d2b415 HDDS-13234. Expired secret key can abort leader OM startup. (apache#8601) d9049a2 HDDS-13220. Change Recon 'Negative usedBytes' message loglevel to DEBUG (apache#8648) 6df3077 HDDS-9223. Use protobuf for SnapshotDiffJobCodec (apache#8503) a7fc290 HDDS-13236. Change Table methods not to throw IOException. (apache#8645) 9958f5b HDDS-13287. Upgrade commons-beanutils to 1.11.0 due to CVE-2025-48734 (apache#8646) 48aefea HDDS-13277. [Docs] Native C/C++ Ozone clients (apache#8630) 052d912 HDDS-13037. Let container create command support STANDALONE , RATIS and EC containers (apache#8559) 90ed60b HDDS-13279. Skip verifying Apache Ranger binaries in CI (apache#8633) 9bc53b2 HDDS-11513. All deletion configurations should be configurable without restart (apache#8003) ac511ac HDDS-13259. Deletion Progress - Grafana Dashboard (apache#8617) 3370f42 HDDS-13246. Change `<? extend KeyValue>` to `<KeyValue>` in hadoop-hdds (apache#8631) 7af8c44 HDDS-11454. Ranger integration for Docker Compose environment (apache#8575) 5a3e4e7 HDDS-13273. Bump awssdk to 2.31.63 (apache#8626) 77138b8 HDDS-13254. Change table iterator to optionally read key or value. (apache#8621) ce288b6 HDDS-13265. Simplify the page Access Ozone using HTTPFS REST API (apache#8629) 36fe888 HDDS-13275. Improve CheckNative implementation (apache#8628) d38484e HDDS-13274. Bump sqlite-jdbc to 3.50.1.0 (apache#8627) 3f3ec43 HDDS-13266. `ozone debug checknative` to show OpenSSL lib (apache#8623) 8983a63 HDDS-13272. Bump junit to 5.13.1 (apache#8625) a927113 HDDS-13271. [Docs] Minor text updates, reference links. (apache#8624) 7e77058 HDDS-13112. [Docs] OM Bootstrap can also happen when follower falls behind too much. (apache#8600) fd13300 HDDS-10775. Support bucket ownership verification (apache#8558) 3ecf345 HDDS-13207. [Docs] Third party systems compatible with Ozone S3. (apache#8584) ad5a507 HDDS-13035. SnapshotDeletingService should hold write locks while purging deleted snapshots (apache#8554) 38a9186 HDDS-12637. Increase max buffer size for tar entry read/write (apache#8618) f31c264 HDDS-13045. Implement Immediate Triggering of Heartbeat when Volume Full (apache#8590) 0701d6a HDDS-13248. Remove `ozone debug replicas verify` option --output-dir (apache#8612) ca1afe8 HDDS-13257. Remove separate split for shell integration tests (apache#8616) 5d6fe94 HDDS-13216. Standardize Container[Replica]NotFoundException messages (apache#8599) 1e47217 HDDS-13168. Fix error response format in CheckUploadContentTypeFilter (apache#8614) 6d4d423 HDDS-13181. Added metrics for internal Snapshot Operations. (apache#8606) 4a461b2 HDDS-10490. Intermittent NPE in TestSnapshotDiffManager#testLoadJobsOnStartUp (apache#8596) bf29f7f HDDS-13235. The equals/hashCode methods in anonymous KeyValue classes may not work. (apache#8607) 6ff3ad6 HDDS-12873. Improve ContainerData statistics synchronization. (apache#8305) 09d3b27 HDDS-13244. TestSnapshotDeletingServiceIntegrationTest should close snapshots after deleting them (apache#8611) 931bc2d HDDS-13243. copy-rename-maven-plugin version is missing (apache#8605) 3b5985c HDDS-13244. Disable TestSnapshotDeletingServiceIntegrationTest 6bf009c HDDS-12927. metrics and log to indicate datanode crossing disk limits (apache#8573) 752da2b HDDS-12760. Intermittent Timeout in testImportedContainerIsClosed (apache#8349) 8c32363 HDDS-13050. Update StartFromDockerHub.md. (apache#8586) ba1887c HDDS-13241. Fix some potential resource leaks (apache#8602) bbaf71e HDDS-13130. Rename all instances of Disk Usage to Namespace usage (apache#8571) 0628386 HDDS-13142. Correct SCMPerformanceMetrics for delete operation. (apache#8592) 516bc96 HDDS-13148. [Docs] Update Transparent Data Encryption doc. (apache#8530) 5787135 HDDS-13229. [Doc] Fix incorrect CLI argument order in OM upgrade docs (apache#8598) ba95074 HDDS-13107. Support limiting output of `ozone admin datanode list` (apache#8595) e7f5544 HDDS-13171. Replace pipelineID if nodes are changed (apache#8562) 3c9d4d8 HDDS-13103. Correct transaction metrics in SCMBlockDeletingService. (apache#8516) f62eb8a HDDS-13160. Remove SnapshotDirectoryCleaningService and refactor AbstractDeletingService (apache#8547) b46e6b2 HDDS-13150. Fixed SnapshotLimitCheck when failures occur. (apache#8532) 203c1d3 HDDS-13206. Update documentation for Apache Ranger (apache#8583) 2072ef0 HDDS-13214. populate-cache fails due to unused dependency (apache#8594) Conflicts: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingTask.java
What changes were proposed in this pull request?
This PR implements the feature described in AWS S3 documentation on bucket owner condition.
Since Ozone does not have an AWS Account ID (12-digit numeric value), the bucket owner's name is used for ownership validation instead.
The implementation covers the following operations:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10775
How was this patch tested?
This patch was tested using unit tests and integration tests.
https://github.com/hevinhsu/ozone/actions/runs/15431261432